Share native and plugin question forms with compact interaction UI - #3147
Conversation
4e0f05b to
6e3a522
Compare
|
🚨 SLOP COP 🚨 · I’m reviewing this pull request now. I’ll check security, general code quality and architectural duplication, performance, and the end-to-end question interaction flow, then return with one synthesized review. |
There was a problem hiding this comment.
🚨 SLOP COP 🚨 · review
Plain-English summary: this PR replaces two separate question-form implementations with one shared form and one compact interaction shell. Native provider questions and plugin-provided questions now use the same selections, free-text answers, previews, keyboard shortcuts, collapse behavior, and visual treatment, while keeping their existing submission adapters.
No blocking findings. The security review found no new injection, authority, validation, or interaction-resolution risks. The production architecture now removes the native/plugin duplication cleanly, and the bounded render/state work introduces no meaningful performance regression.
One low-severity follow-up: plans/bb-mobile-research/thread-view-promptbox.md lines 18 and 106 still reference the deleted app-local user-question-form-state.ts and describe that state as app-local. Please update the mobile reuse inventory to point to packages/shared-ui/src/components/ui/question-form-state.ts and question-form.tsx, with PendingInteractionShell.tsx and ThreadQuestionFormHost.tsx called out as app-specific integration. This is documentation drift, not a merge blocker.
Verification on 69b7da8dce789b46ad22f925debdeefb06a7757b:
- Started an isolated source dev app with a fresh store and drove the real fallback Ask User Question plugin through Doobie. A two-question card rendered in the compact shell; numbered shortcuts selected single- and multi-select answers; the selected option exposed its multiline preview; collapse/re-expand preserved the active question and selections; submit removed the pending interaction; and the waiting agent received exactly
Layout: SpaciousandExtras: Keyboard shortcuts, Option previews. - The native Claude Code live path was attempted but blocked before rendering by an expired OAuth session. Native/shared behavior was still covered by source tracing and the passing app suite.
@bb/appand@bb/shared-ui: 481 test files passed, 3,934 tests passed, 4 skipped.bb-plugin-ask-user-question: 30 tests passed. The changed plugin-app build test passed all 11 tests. A broader local plugin-build run also hit two unchanged path-string assertions caused by macOS/tmpresolving to/private/tmp; those files are outside this PR diff, and head CI is green.- Focused Turbo typechecks passed for the app, shared UI, plugin, and plugin build. Lint completed with 0 errors.
git diff --checkpassed. - The verification inventory still reports the already-known unrelated unmapped
browserCLI family. All required CI checks on the reviewed SHA are green.
Human comments
What was wrong
Built-in questions and the ask-user-question plugin duplicated their form UI, state, and shortcut handling. Their containers also diverged: built-in prompts had an orange border and collapse controls while plugin forms used a neutral border without collapse controls. Compact permission prompts wrapped across multiple lines on mobile.
What changed
PendingInteractionShellfor built-in questions, approvals, plan reviews, and plugin forms: neutral border, attention dot, title/caret disclosure controls, and a single-line compact mobile layout.QuestionFormand answer-state implementation for built-in and plugin questions, including tabs, single/multiple selection, Other/free text, validation, navigation, and option previews. Delete the duplicate implementations and consolidate form tests.user_answerresolution and cancellation stops the thread; the plugin submits{answers}and retains its interaction cancellation behavior.How you verified
pnpm exec turbo run typecheck lint --filter=@bb/app --filter=@bb/shared-ui --filter=bb-plugin-ask-user-question --filter=@bb/plugin-buildpassed. Formatting and diff checks passed.pnpm start:worktree; app responds HTTP 200 and the host daemon connects.browserCLI family.Screenshots
Before / After screenshot gallery — 19 surfaces/states × desktop/mobile × compact/expanded, compared with PR base
6cdb4ba612. Both columns use the same fixture data and viewport sizes; select a surface to compare it directly. Plugin forms without an earlier compact mode show their actual expanded Before view with an explicit label. Each image opens at full size.All 38 compact captures measure 38px high with no horizontal overflow. This includes built-in and plugin questions, command/file/tool permissions, child-thread approvals, plan review, resolving requests, secrets, unavailable plugins, errors, Other answers, and free-text drafts. Compact errors use a red dot without competing with the title; expanded errors show the full message.
Compact mode now shows the short label (for example, Approval needed), dot, and caret; details, source links, and decisions appear after expansion. Expanded headings remain readable, with child-thread links truncating first. Attention dots align with the first line when headings wrap. All 3,935 app tests passed (3 skipped), along with typecheck and lint.
Browser captures use synthetic Ladle previews at 1280px and 390px viewport widths; mobile is Chromium emulation. No credentials entered. Following the compact-error change, 20 shell/controller tests, app typecheck, and lint passed.